Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fetch docs with git clone instead of GitHub API #4000

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Dylan-fa
Copy link
Contributor

Docs folder now fetched with git clone and sparse checkout. Tests updated as well to mock local repo docs.

Docs folder now fetched with git clone and sparse checkout.
Tests updated as well to mock local repo docs.
Docs folder now fetched with git clone and sparse checkout.
Tests updated as well to mock local repo docs.
@sengi
Copy link
Contributor

sengi commented Jun 2, 2023

Hello and welcome! Sorry to have kept you waiting so long. Huge thanks for working on this! 🙇 I (or one of my colleagues) will try to get back to you with a proper review as soon as poss.

@sengi sengi self-requested a review June 2, 2023 10:31
@sengi sengi self-assigned this Jun 2, 2023
Copy link
Contributor

@sengi sengi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for this! ❤️

I think the only thing stopping the CI / pre-merge checks from passing is the trivial rubocop error; rubocop -a should sort it out. (At least, the tests passed on my workstation with that change.)

I'll need to find a colleague to give this a second look before merging, but LGTM!

app/github_repo_fetcher.rb Outdated Show resolved Hide resolved
Gemfile Outdated Show resolved Hide resolved
spec/app/github_repo_fetcher_spec.rb Show resolved Hide resolved
@sengi sengi requested a review from ChrisBAshton June 5, 2023 10:07
@Dylan-fa
Copy link
Contributor Author

Dylan-fa commented Jul 7, 2023

Hi @sengi, thanks for the code review, please see my updated change (which should hopefully pass CI now, once the run is approved).

@sengi sengi requested a review from deborahchua July 7, 2023 16:20
@sengi
Copy link
Contributor

sengi commented Jul 7, 2023

Thanks @Dylan-fa! One of my colleagues will hopefully be able to take a look for you soon so we can get that second approval and then hopefully we'll be good to go 🚀

Copy link
Contributor

@ChrisBAshton ChrisBAshton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much for raising this, @Dylan-fa

It's looking good (with some real improvements to our tests!). I've just got some minor comments to address below. I'll then give it a quick manual test before approval 👍

PS: if possible, please squash all your commits together; there's not much value in keeping 'fixup' and merge commits.

Comment on lines +1 to +32
# Lorem ipsum

## tl;dr

Lorem ipsum dolor sit amet, consectetur adipiscing elit. Nam efficitur lacus in
neque consequat porta, [Suspendisse iaculis](#suspendisse-iaculis). Sed et
[inline link](./inline-link.md), imperdiet eros ut, [aliased link].
[Absolute link](https://nam.com/eget/dui/absolute-link.md)

[localhost](localhost:999)

[Relative docs link with period](./docs/prefixed.md)

[Relative docs link without period](docs/no-prefix.md)

[Link relative to root](/public/json_examples/requests/foo.json)

[Subfolder](docs/some-subfolder/foo.md)

Visit “http://localhost:3108”

## Suspendisse iaculis

![Suspendisse iaculis](suspendisse_iaculis.png)

[aliased link]: ./lib/aliased_link.rb

## Other headings to test

### data.gov.uk

### Patterns & Style Guides
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The contents of this file aren't under test, as far as I can see. I think we can just empty the file (or replace the contents with just a string like "placeholder text"). This will make it clearer to devs that the two spec fixture files don't need to be kept in sync.

instance.instance_variable_set(:@client, temporary_client)
yield
instance.instance_variable_set(:@client, before_client)
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very glad to see this method go - thank you for improving the tests! ⭐

Comment on lines +104 to +105
relative_path: relative,
source_url: url,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: I'd rename your relative and url variables to relative_path and source_url. Then the above two lines can be reduced to:

Suggested change
relative_path: relative,
source_url: url,
relative_path:,
source_url:,

Should be easier to follow, as there are fewer keys/variable names to keep track of.

def data_for_github_doc(doc_path, repo_name)
contents = File.read(doc_path)

path = Pathname.new(doc_path)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you'll need to add require "pathname" on line 3 to use this.

docs_path = doc.path.sub("docs/", "").match(/(.+)\..+$/)[1]
filename = docs_path.split("/")[-1]
title = ExternalDoc.title(contents) || filename
def data_for_github_doc(doc_path, repo_name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could give this parameter a better name:

Suggested change
def data_for_github_doc(doc_path, repo_name)
def data_for_github_doc(absolute_path_to_doc, repo_name)

Comment on lines +92 to +96
docs_path_with_extension = path.each_filename.to_a.drop_while { |f| f != "docs" }.drop(1).join("/")
docs_path_without_extension = docs_path_with_extension.chomp(".md")
relative = path.each_filename.to_a.drop_while { |f| f != "docs" }.join("/")

basename_without_extension = File.basename(doc_path, ".md")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, you only use docs_path_with_extension to form docs_path_without_extension. I think we can cut out the middle-man and set all the variables we need up front a bit more cleanly:

Suggested change
docs_path_with_extension = path.each_filename.to_a.drop_while { |f| f != "docs" }.drop(1).join("/")
docs_path_without_extension = docs_path_with_extension.chomp(".md")
relative = path.each_filename.to_a.drop_while { |f| f != "docs" }.join("/")
basename_without_extension = File.basename(doc_path, ".md")
relative_path = Pathname.new(absolute_path_to_doc)
.each_filename
.to_a
.drop_while { |f| f != "docs" }
.join("/")
docs_path_without_extension = relative_path.delete_prefix("docs/").chomp(".md")
basename_without_extension = docs_path_without_extension.split("/").last

NB there's also no need for therelative variable anymore, we've already defined relative_path above, which you can refer to on line 104.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants